replaceVarsWith: init#360771
Conversation
github-actions
Bot
added
6.topic: nixos
wolfgangwalther
force-pushed
the
substitute-all-replace-vars-with
branch
2 times, most recently
from
3e8137e to
0147123
Compare
|
Welcome to the committers, @wolfgangwalther! Glad to have you here, and thanks as always for your high quality and well-thought-out PRs and collaboration. |
ofborg
Bot
requested review from
Artturin,
adevress,
amiloradovsky,
bjornfor,
edolstra,
gebner,
primeos and
prusnak
ofborg
Bot
added
10.rebuild-darwin: 501+
|
Looking through the current merge conflict I see #360576 - which would be an alternative way of dealing with those cases, without introducing Opinions, anyone? |
wolfgangwalther
force-pushed
the
substitute-all-replace-vars-with
branch
from
0147123 to
15719f9
Compare
There was a problem hiding this comment.
I should be able to give this a thorough review this week. Thank you for your patience with the silence so far, @wolfgangwalther.
The interface I expected for replaceVarsWith was an adapted stdenv.mkDerivation, rather than a function of three arguments. I'm open to a function of three arguments, but I need to wrap my head around it.
The short version I had in mind was:
{ replaceVarsWith }:
path: attrs:
replaceVarsWith {
src = path;
replacements = attrs;
}
It's sort of fundamentally odd to me for a With variant to have anything other than one attrset as its input.
Regarding #360576, it's sort of a brute-force approach, and I don't like it as much as I like a general-purpose replaceVarsWith.
That works for me, too. I'm not bound to the current interface at all - I'm all ears for alternative suggestions. |
Hm. For me it's the opposite. For example, compare Now the question whether partial applications is any useful in this case... probably not. I also found another example: |
wolfgangwalther
force-pushed
the
substitute-all-replace-vars-with
branch
2 times, most recently
from
93b9dfa to
340ed46
Compare
I tried the approach with one attrset as argument in a fixup commit. I like it more as well. It also makes the diff nicer to read.
I put lsb-release back in and moved it to the new replaceVarsWith as well. |
github-actions
Bot
removed
10.rebuild-darwin: 501+
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
I like it a lot. One request for the copy-and-paste machine to come out, then I'd like to merge. This looks great in all the users.
|
If the PR is just the |
Nope, see #357395, which caused a lot of darwin rebuilds, too. Since |
Takes the extended features of nix substituteAll to a replaceVars variant to get rid of those cases that use substituteAll to build a full package with meta information etc.
wolfgangwalther
force-pushed
the
substitute-all-replace-vars-with
branch
from
340ed46 to
4ce241c
Compare
|
Looks great, tastes great. Want to use your new committer powers to push the button? |
2 participants
Working my way towards removing substituteAll in #237216.
This takes the extended features of nix substituteAll to a replaceVars variant to get rid of those cases that use substituteAll to build a full package with meta information etc.
There are at least 15 more packages which will make use of
replaceVarsWith, but I didn't include the changes here to keep the number of notifications lower while we iterate on the function itself. All those other cases that I currently know of useisExecutable = true.Part of my overall effort for structuredAttrs in #205690.
@philiptaron @emilazy
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.